Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stack probe CFI information for amd64 and i386 #8848

Merged
merged 2 commits into from Jul 31, 2019

Conversation

ctk21
Copy link
Contributor

@ctk21 ctk21 commented Jul 30, 2019

This PR fixes caml_c_call and caml_call_gc to have the right CFI information when the stack probe is being done on amd64 and i386.

This problem was discovered using perf on Linux to profile OCaml binaries and seeing bad call-stacks in some of the caml_c_call samples.

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the code. It looks good to me.

Could you add an entry to the Changes file? Otherwise one of us will do it.

@ctk21
Copy link
Contributor Author

ctk21 commented Jul 31, 2019

Could you add an entry to the Changes file? Otherwise one of us will do it.

Thanks for the review. I've updated the Changes file.

@xavierleroy
Copy link
Contributor

Perfect, thank you!

@xavierleroy xavierleroy merged commit 4cd8dd2 into ocaml:trunk Jul 31, 2019
@dra27
Copy link
Member

dra27 commented Jul 31, 2019

Is there any reason not to put this in 4.09 (I'll do the adjust and cherry-pick, if so) - @xavierleroy? @Octachron?

@xavierleroy
Copy link
Contributor

This would be good to have in 4.09, agreed.

dra27 added a commit that referenced this pull request Jul 31, 2019
dra27 pushed a commit that referenced this pull request Jul 31, 2019
Add CFI information for the stack probes for amd64 and i386 when doing caml_c_call and caml_call_gc.

(cherry picked from commit 4cd8dd2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants